Skip to content

fix: disable xembed tray selection manager registeration#459

Open
BLumia wants to merge 1 commit into
linuxdeepin:masterfrom
BLumia:no-tray-sm
Open

fix: disable xembed tray selection manager registeration#459
BLumia wants to merge 1 commit into
linuxdeepin:masterfrom
BLumia:no-tray-sm

Conversation

@BLumia
Copy link
Copy Markdown
Member

@BLumia BLumia commented May 18, 2026

The current FdoSelectionManager is for preparing the legacy xembed2sni solution. We currently no longer plans to go with such solution, but the current code will register _NET_SYSTEM_TRAY_S0. This patch disabled related code path so it won't gets registered. We should do registeration from dde-daemon instead.

注:此patch并不直接解决下面关联的PMS问题单,只是有相关性,是相关工作的
一部分.

PMS: BUG-360349

@BLumia BLumia requested a review from tsic404 May 18, 2026 08:55
Copy link
Copy Markdown

@sourcery-ai sourcery-ai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry @BLumia, you have reached your weekly rate limit of 500000 diff characters.

Please try again later or upgrade to continue using Sourcery

The current FdoSelectionManager is for preparing the legacy xembed2sni
solution. We currently no longer plans to go with such solution, but
the current code will register `_NET_SYSTEM_TRAY_S0`. This patch
disabled related code path so it won't gets registered. We should
do registeration from dde-daemon instead.

注:此patch并不直接解决下面关联的PMS问题单,只是有相关性,是相关工作的
一部分.

PMS: BUG-360349
Log:
@deepin-ci-robot
Copy link
Copy Markdown

deepin pr auto review

你好!我是CodeGeeX,你的智能编程助手。我已经仔细审查了你提供的 Git Diff。

这次修改的主要目的是通过宏 _DDE_TRAY_LOADER_HANDLE_XEMBED_SELECTION_MANAGER 来条件编译 FdoSelectionManager 相关的代码,以便在特定环境下(如由 dde-daemon 处理时)关闭该功能。同时更新了版权声明。

以下是我从语法逻辑、代码质量、代码性能和代码安全四个维度提出的审查意见和改进建议:

1. 语法与逻辑

  • 宏的命名与定义规范
    • 问题:代码中使用 #define _DDE_TRAY_LOADER_HANDLE_XEMBED_SELECTION_MANAGER 0 来定义宏,并在后续使用 #if _DDE_TRAY_LOADER_HANDLE_XEMBED_SELECTION_MANAGER。在 C++ 中,以单下划线加大写字母(如 _DDE_...)开头的标识符是保留给编译器和标准库实现的。这可能会导致不可预见的宏冲突或未定义行为。
    • 建议:将宏名更改为非保留命名,例如 DDE_TRAY_LOADER_HANDLE_XEMBED_SELECTION_MANAGER,或者使用项目统一的前缀(如 DDE_TRAY_...)。
  • 宏的定义位置与可见性
    • 问题:该宏直接定义在头文件 xembedprotocolhandler.h 中。这意味着所有包含此头文件的代码都会受到这个硬编码宏的影响。如果未来需要根据不同的编译目标动态切换此功能,修改头文件是不够灵活的。
    • 建议:最佳实践是通过构建系统(如 CMake 的 add_definitionsconfigure_file)来传递此类特性宏,而不是在头文件中硬编码为 0。如果在当前架构下必须在头文件中定义,请确保它被放置在头文件保护符之内,并且考虑是否应该使用 #ifndef 来允许编译命令覆盖它。
    • 改进代码示例
      // 允许编译参数优先,如果没有定义则默认为 0
      #ifndef DDE_TRAY_LOADER_HANDLE_XEMBED_SELECTION_MANAGER
      #define DDE_TRAY_LOADER_HANDLE_XEMBED_SELECTION_MANAGER 0
      #endif
  • 潜在的悬空指针风险(逻辑漏洞)
    • 问题:在头文件中,class FdoSelectionManager; 的前置声明依然存在。如果在 .cpp 中因为宏为 0 而没有包含 fdoselectionmanager.h,且未来有人在没有宏保护的情况下误用了 FdoSelectionManager 的具体类型(例如调用其方法),会导致编译失败。更严重的是,如果 m_selectionManager 成员变量(虽然 diff 中未展示,但推测存在)没有被宏保护,在宏为 0 时,该指针将永远不会被初始化,如果其他未受保护的代码解引用了它,将引发崩溃。
    • 建议:确保类成员变量 FdoSelectionManager *m_selectionManager; 也必须被 #if 包裹,或者在宏为 0 时将其定义为 nullptr 且不实例化。

2. 代码质量

  • 注释风格
    • 问题:头文件中新增的注释使用了 C 风格的 /* ... */ 或普通行注释,解释了为什么不需要这个宏。这对于理解代码上下文非常有帮助,但注释稍微有些冗长。
    • 建议:注释内容很好,建议保留。但可以稍微精简,使其更易于维护者阅读。
    • 改进示例
      // FDO selection manager registration is handled by dde-daemon.
      // If enabled here, tray context menu positioning must also be handled correctly.
      // Consider moving FdoSelectionManager to dde-shell in the future.
      #define DDE_TRAY_LOADER_HANDLE_XEMBED_SELECTION_MANAGER 0
  • 环境检测逻辑的完整性
    • 问题:在 .cpp 的条件判断中:if ((qgetenv("XDG_SESSION_TYPE") == "wayland") && UTIL->isXAvaliable())。这里 isXAvaliable 存在拼写错误(应为 isXAvailable,多了一个 'i',虽然这是旧代码,但既然审查到,建议顺手修复)。
    • 建议:修正拼写错误 isXAvaliable -> isXAvailable

3. 代码性能

  • 环境变量的获取
    • 问题qgetenv("XDG_SESSION_TYPE") 会在堆上分配 QByteArray 来比较字符串。在构造函数中调用一次对性能影响微乎其微,但如果该类被频繁实例化,或者未来这段逻辑被移动到循环中,就会产生不必要的内存分配和字符串比较开销。
    • 建议:当前实现可接受。若后续有性能优化需求,可考虑使用 qEnvironmentVariableIsEmptyqEnvironmentVariableIntValue(如果环境变量是整数标识),或者使用 X11 的原生 API 检测会话类型,避免 QByteArray 的构造。

4. 代码安全

  • 宏为 0 时的降级安全
    • 问题:当 _DDE_TRAY_LOADER_HANDLE_XEMBED_SELECTION_MANAGER 为 0 时,Wayland 环境下将不再注册 FDO selection manager。如果 dde-daemon 没有如预期般接管这部分工作,系统托盘的 XEmbed 嵌入功能可能会静默失败,用户无法看到托盘图标,且没有任何错误提示。
    • 建议:在 #else 分支中增加调试日志,明确提示开发者/用户当前未注册 FDO Selection Manager。
    • 改进代码示例
      #if DDE_TRAY_LOADER_HANDLE_XEMBED_SELECTION_MANAGER
          if ((qgetenv("XDG_SESSION_TYPE") == "wayland") && UTIL->isXAvailable()) {
              qWarning() << "Not running on X11, registering FDO selection manager";
              m_selectionManager = new FdoSelectionManager(this);
          }
      #else
          if (qgetenv("XDG_SESSION_TYPE") == "wayland") {
              qDebug() << "FDO selection manager registration is disabled, expecting dde-daemon to handle it.";
          }
      #endif

综合修改建议

结合以上分析,我为你提供一份优化后的代码片段:

xembedprotocolhandler.h:

// SPDX-FileCopyrightText: 2024 - 2026 UnionTech Software Technology Co., Ltd.
//
// SPDX-License-Identifier: GPL-3.0-or-later

#ifndef XEMBEDPROTOCOLHANDLER_H
#define XEMBEDPROTOCOLHANDLER_H

#include <xcb/composite.h>
#include <xcb/xcb_image.h>

// FDO selection manager registration is handled by dde-daemon.
// If enabled here, tray context menu positioning must also be handled correctly.
// Consider moving FdoSelectionManager to dde-shell in the future.
#ifndef DDE_TRAY_LOADER_HANDLE_XEMBED_SELECTION_MANAGER
#define DDE_TRAY_LOADER_HANDLE_XEMBED_SELECTION_MANAGER 0
#endif

#if DDE_TRAY_LOADER_HANDLE_XEMBED_SELECTION_MANAGER
class FdoSelectionManager;
#endif

namespace tray {
class XembedProtocol : public AbstractTrayProtocol, public QAbstractNativeEventFilter
{
    // ...
#if DDE_TRAY_LOADER_HANDLE_XEMBED_SELECTION_MANAGER
    FdoSelectionManager *m_selectionManager = nullptr;
#endif
};
}
#endif // XEMBEDPROTOCOLHANDLER_H

xembedprotocolhandler.cpp:

#include "abstracttrayprotocol.h"
#include "traymanager1interface.h"
#include "xembedprotocolhandler.h"

#if DDE_TRAY_LOADER_HANDLE_XEMBED_SELECTION_MANAGER
#include "fdoselectionmanager.h"
#endif

#include "util.h"
#include "plugin.h"

// ... 其他代码 ...

XembedProtocol::XembedProtocol(QObject *parent)
    : AbstractTrayProtocol(parent)
{
    // ... 其他初始化 ...

    QMetaObject::invokeMethod(this, &XembedProtocol::onTrayIconsChanged, Qt::QueuedConnection);

#if DDE_TRAY_LOADER_HANDLE_XEMBED_SELECTION_MANAGER
    // 注意:修复了 isXAvaliable 的拼写错误 -> isXAvailable
    if ((qgetenv("XDG_SESSION_TYPE") == "wayland") && UTIL->isXAvailable()) {
        qWarning() << "Not running on X11, registering FDO selection manager";
        m_selectionManager = new FdoSelectionManager(this);
    }
#else
    if (qgetenv("XDG_SESSION_TYPE") == "wayland") {
        qDebug() << "Skipping FDO selection manager registration (disabled by macro).";
    }
#endif

    QTimer::singleShot(0, this, [this](){
        m_trayManager->Manage();
    });
}

希望这些审查意见和代码改进能对你有所帮助!如果有任何疑问,欢迎随时提问。

@deepin-ci-robot
Copy link
Copy Markdown

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: BLumia, tsic404

The full list of commands accepted by this bot can be found here.

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants